Skip to content

Change GeoIpCache and EnrichCache to LongAdder #132922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Aug 20, 2025
Merged

Change GeoIpCache and EnrichCache to LongAdder #132922

merged 9 commits into from
Aug 20, 2025

Conversation

szybia
Copy link
Contributor

@szybia szybia commented Aug 14, 2025 •
edited by joegallo
Loading

From the javadocs:

[LongAdder] is usually preferable to AtomicLong when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.

@szybia szybia added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Aug 14, 2025
@szybia szybia requested a review from joegallo August 14, 2025 13:14
@szybia szybia marked this pull request as ready for review August 14, 2025 13:14
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Aug 14, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine
Copy link
Collaborator

Hi @szybia, I've created a changelog YAML for you.

@joegallo
Copy link
Contributor

There's more prior art (not a ton, but some) for using a CounterMetric for tracking times in the Elasticsearch codebase than for using a LongAdder directly.

joegallo@simulacron:~/Code/elastic/elasticsearch $ git grep 'new LongAdder' | grep -i time | grep -i -E '(nanos|millis)' | grep -iv test/
x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/input/DirectBlobContainerIndexInput.java:            private final LongAdder timeNanos = new LongAdder();
joegallo@simulacron:~/Code/elastic/elasticsearch $ git grep 'new CounterMetric()' | grep -i time | grep -i -E '(nanos|millis)' | grep -iv test/
server/src/main/java/org/elasticsearch/index/engine/Engine.java:        private final CounterMetric throttleTimeMillisMetric = new CounterMetric();
server/src/main/java/org/elasticsearch/ingest/IngestMetric.java:    private final CounterMetric ingestTimeInNanos = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric snapshotRateLimitingTimeInNanos = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric restoreRateLimitingTimeInNanos = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric uploadTimeInMillis = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric uploadReadTimeInNanos = new CounterMetric();
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsStatsAction.java:        CounterMetric ingestTimeInMillis = new CounterMetric();

Is there any overriding reason to be unique here, or does it make more sense to just do what we usually do?

@joegallo
Copy link
Contributor

I'd be fine with +1ing the type changes by themselves, but in order to change the logic on the time math, I'm going to have to start by understanding the reason this fence was here in the first place.

szybia added 2 commits August 19, 2025 12:35
@szybia
remove timing changes
b9b4ed4
@szybia
Merge remote-tracking branch 'upstream/main' into geoip-enrich-cache-…
730dad7 
…improv

* upstream/main: (92 commits)
  ESQL: mark LOOKUP JOIN as ExecutesOn.Any by default (elastic#133064)
  Fix 404s in REST API landing page (elastic#133086)
  Fix release tests for OptimizerVerificationTests (elastic#133100)
  Make Glob non-recursive (elastic#132798)
  Update ES|QL function list for release versions (elastic#133096)
  Split transport version func test into abstract base (elastic#133035)
  Omit project ID from snapshot metrics (elastic#133098)
  Mute org.elasticsearch.xpack.esql.analysis.AnalyzerTests testNoDenseVectorFailsForMagnitude elastic#133013
  Mute org.elasticsearch.xpack.esql.optimizer.OptimizerVerificationTests testRemoteEnrichAfterCoordinatorOnlyPlans elastic#133015
  Mute org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT test {p0=search/160_exists_query/Test exists query on _id field} elastic#133097
  Rename initial to unreferenced in transport versions (elastic#133082)
  Rename exception type header (elastic#133045)
  ESQL: Pluggable tests for Operator status (elastic#132876)
  ESQL: Mark new signatures in MIN and MAX (elastic#132980)
  Don't try to serialize half-baked cluster info (elastic#132756)
  migrate ml_rollover_legacy_indices transport version (elastic#133008)
  Enable `exclude_source_vectors` by default for new indices (elastic#131907)
  Expose APIs needed by flush during translog replay (elastic#132960)
  Change reporting_user role to leverage reserved kibana privileges (elastic#132766)
  Update TasksIT for batched execution (elastic#132762)
  ...
@szybia szybia changed the title GeoIpCache and EnrichCache improvements Change GeoIpCache and EnrichCache to LongAdder Aug 19, 2025
szybia added 2 commits August 19, 2025 12:54
@szybia
update changelog
9c8656a
@szybia
Merge remote-tracking branch 'upstream/main' into geoip-enrich-cache-…
b810b0d 
…improv

* upstream/main: (58 commits)
  Fixing flaky LoggedExec (tests) (elastic#133215)
  CPS search should not use `skip_unavailable` (elastic#132927)
  Don't fail search if bottom doc can't be formatted (elastic#133188)
  Mute org.elasticsearch.xpack.esql.action.RandomizedTimeSeriesIT testGroupByNothing elastic#133225
  Mute org.elasticsearch.index.codec.tsdb.es819.ES819TSDBDocValuesFormatTests testSortedSetDocValuesWithSkipperSmall elastic#133224
  Mute org.elasticsearch.index.codec.tsdb.es819.ES819TSDBDocValuesFormatTests testSortedNumberMergeAwayAllValuesWithSkipper elastic#133223
  Adding support for index.number_of_replicas to data stream settings (elastic#132748)
  Mute org.elasticsearch.index.codec.tsdb.es819.ES819TSDBDocValuesFormatTests testSortedDocValuesSingleUniqueValue elastic#133221
  Fix VectorSimilarityFunctionsIT (elastic#133206)
  Mute org.elasticsearch.xpack.esql.action.RandomizedTimeSeriesIT testGroupBySubset elastic#133220
  Increase the number of FORK branches in ForkGenerator (elastic#132019)
  Mute org.elasticsearch.index.mapper.blockloader.IpFieldBlockLoaderTests testBlockLoader {preference=Params[syntheticSource=true, preference=STORED]} elastic#133218
  Mute org.elasticsearch.index.mapper.blockloader.IpFieldBlockLoaderTests testBlockLoader {preference=Params[syntheticSource=true, preference=DOC_VALUES]} elastic#133217
  Mute org.elasticsearch.index.mapper.blockloader.IpFieldBlockLoaderTests testBlockLoader {preference=Params[syntheticSource=true, preference=NONE]} elastic#133216
  Set default processor allocation for test clusters (elastic#133204)
  Add mapper for exponential histograms (elastic#132493)
  Fix offset handling in Murmur3Hasher (elastic#133193)
  unmute testDoesNotResolveClosedIndex (elastic#133115)
  Fix an AWS SDK v2 release note (elastic#133155)
  Limit the depth of a filter (elastic#133113)
  ...
@szybia szybia added >bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP auto-backport Automatically create backport pull requests when merged v8.18.6 v8.19.3 v9.1.4 v9.0.7 and removed >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team labels Aug 20, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Aug 20, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine
Copy link
Collaborator

Hi @szybia, I've updated the changelog YAML for you.

@szybia szybia enabled auto-merge (squash) August 20, 2025 17:13
@szybia szybia merged commit 92d69b5 into elastic:main Aug 20, 2025
34 checks passed
@szybia szybia mentioned this pull request Aug 20, 2025
Merged
szybia added a commit to szybia/elasticsearch that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (elastic#132922)
86135b1 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
This was referenced Aug 20, 2025
Merged
Merged
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 8.18
✅ 8.19
✅ 9.1
✅ 9.0

szybia added a commit to szybia/elasticsearch that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (elastic#132922)
728f91a 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
szybia added a commit to szybia/elasticsearch that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (elastic#132922)
7ff8f4a 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
szybia added a commit to szybia/elasticsearch that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (elastic#132922)
66ab3c8 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
elasticsearchmachine pushed a commit that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (#132922) (#133250)
4636f4d 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
elasticsearchmachine pushed a commit that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (#132922) (#133248)
1a584b0 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
elasticsearchmachine pushed a commit that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (#132922) (#133249)
54af22b 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
elasticsearchmachine pushed a commit that referenced this pull request Aug 20, 2025
@szybia
Change GeoIpCache and EnrichCache to LongAdder (#132922) (#133251)
9cec1b3 
From the javadocs:

> [`LongAdder`] is usually preferable to `AtomicLong` when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control.

Since we largely write stats and read very infrequently, let's avoid any lock-contention possibilities by switching to the more appropriate data type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joegallo joegallo joegallo approved these changes

Assignees

No one assigned

Labels

auto-backport Automatically create backport pull requests when merged >bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v8.18.6 v8.19.3 v9.0.7 v9.1.4 v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@szybia @elasticsearchmachine @joegallo